add client cert support #70
Conversation
Signed-off-by: Ben Ritcey <[email protected]>
|
Looks like it's the use of the deprecated client.Authentication.SetBasicAuth that's causing tests to fail: cmd/jiralert/main.go:118:4: SA1019: client.Authentication.SetBasicAuth is deprecated: Use BasicAuthTransport instead (staticcheck) but AFAICT you can't use BasicAuthTransport with TLSConfig, etc. I will, however, confirm that with the go-jira project. |
examples/jiralert.yml
Outdated
| # optional client cert | ||
| # cert_file: /path/to/cert.crt | ||
| # key_file: /path/to/cert.key | ||
| # insecure_skip_verify: false |
There was a problem hiding this comment.
This should be default, so let's remove it
There was a problem hiding this comment.
Do we want to indicate that insecure_skip_verify is an option in the config, e.g.,
# insecure_skip_verify: true
?
| // want to be able to override CAFile/KeyFile/CertFile in each receiver | ||
| // while still having a default value - use "none" to indicate no value | ||
| // set | ||
| if rc.CAFile == "none" { |
There was a problem hiding this comment.
none? hmmm, don't get this, why we need this?
There was a problem hiding this comment.
This is what I was referring to in the first bullet point of the PR comment -
if you set a CAFile (or KeyFile/CertFile) in the defaults but didn't want it set in one of the specific receivers, you can't just set it to blank in the receiver config - the current config logic is that "if it's blank in the receiver config, use the default".
So I went with a "magic" value of "none" - I don't love that sort of magic, but I'm not sure what else to do short of totally rewriting the config logic. AFAICT, you can't set 'nil' in YAML - setting it to 'null' or '~' just results in an empty string.
There was a problem hiding this comment.
I would expect the following to work:
Marshal the default config
Then marshal the receiver config into the initialized default struct - all fields that are missing in the receiver's YAML will not override the defaults. All fields that are present in the receiver's config will override the default config.
Example receiver uses the defaults:
defaults:
api_url: url
cert_file: file
receivers:
api_url: url2
Receiver Parsed:
Receiver{
CAFile = file
APIURL = url2
Example receiver overrides the defaults:
defaults:
api_url: url
cert_file: file
receivers:
cert_file: file2
api_url: url2
Receiver Parsed:
Receiver{
CAFile = file2
APIURL = url2
Example receiver overrides the defaults and doesn't use cert:
defaults:
api_url: url
cert_file: file
receivers:
cert_file: ""
api_url: url2
Receiver Parsed:
Receiver{
CAFile = ""
APIURL = url2
In other words, when the field doesn't exist in the receiver config use the default and in all other cases use the field value from the receiver config.
Also, let's add a test for this to make it clear and make sure we don't break this logic in the feature
There was a problem hiding this comment.
(apologies for the big gap in responding - other work got in the way)
This may be my Go inexperience showing, but I believe that won't work as-is, since unmarshaling YAML with missing fields just initializes those fields to the empty string - it doesn't differentiate between "not specified" and "blank".
https://play.golang.org/p/_4ih2Xomnit
However, if I make them string pointers, the pointers will get initialized to nil if not set, which I can use to detect "not specified" vs. "blank". This appears to be one of the cases when using string pointers vs. strings makes sense: https://dhdersch.github.io/golang/2016/01/23/golang-when-to-use-string-pointers.html
Let me know if that sounds like a reasonable approach.
Signed-off-by: Ben Ritcey <[email protected]>
| // want to be able to override CAFile/KeyFile/CertFile in each receiver | ||
| // while still having a default value - use "none" to indicate no value | ||
| // set | ||
| if rc.CAFile == "none" { |
There was a problem hiding this comment.
I would expect the following to work:
Marshal the default config
Then marshal the receiver config into the initialized default struct - all fields that are missing in the receiver's YAML will not override the defaults. All fields that are present in the receiver's config will override the default config.
Example receiver uses the defaults:
defaults:
api_url: url
cert_file: file
receivers:
api_url: url2
Receiver Parsed:
Receiver{
CAFile = file
APIURL = url2
Example receiver overrides the defaults:
defaults:
api_url: url
cert_file: file
receivers:
cert_file: file2
api_url: url2
Receiver Parsed:
Receiver{
CAFile = file2
APIURL = url2
Example receiver overrides the defaults and doesn't use cert:
defaults:
api_url: url
cert_file: file
receivers:
cert_file: ""
api_url: url2
Receiver Parsed:
Receiver{
CAFile = ""
APIURL = url2
In other words, when the field doesn't exist in the receiver config use the default and in all other cases use the field value from the receiver config.
Also, let's add a test for this to make it clear and make sure we don't break this logic in the feature
britcey
force-pushed
the
feature/client_certs
branch
2 times, most recently
from
c721010
to
721d928
Compare
…specified Signed-off-by: Ben Ritcey <[email protected]>
britcey
force-pushed
the
feature/client_certs
branch
from
721d928
to
9cf450d
Compare
Signed-off-by: Ben Ritcey [email protected]
This adds the ability to specify client certificates to use when talking to Jira - depending on the config, that may be sufficient to identify the account creating the Jira issues.
I left user/password as required - in the case where the client cert is used for authentication, they'll often just be ignored (depends on server config).
I didn't see a clean way to unset user_cert/user_key in the individual receivers if set in the default, so I made the value of 'none' magical. I'm open to any better suggestions.
what's the correct way to set insecure_skip_verify boolean based on the default? As-is, you'd have to specify it per receiver.